Repository navigation
Add the Test Companion action - #91
naimish-browse wants to merge 29 commits into
Conversation
Lets a team ask BrowserStack Test Companion for test work from a pull request comment. Someone comments "@test-companion test this PR"; it reads the change, posts the tests it would write, and writes them once a person replies "go". The result comes back as one comment and one check. Jira: STM-3663, STM-5434. A composite action with ZERO dependencies and no dist/ bundle — unlike the JavaScript actions already here, which commit 8-15MB bundles by hand. What is in src/ is what runs, and there is nothing to rebuild on release. 422 tests, no network and no credentials needed: cd test-companion && node --test
Picked up from the source repo's security review: these git commands run after the agent, which has --yolo write access to the tree, so a planted .git/hooks/pre-commit would have been executed with the App's private key in its environment.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| with: | ||
| fetch-depth: 0 # jobs that look at what changed need history | ||
|
|
||
| - uses: browserstack/github-actions/test-companion@v1 |
There was a problem hiding this comment.
This one is deliberate, and I would like to keep it.
The line is this action referring to itself, in the starter workflow we ask customers to copy into their own repository. v1 is the floating tag that release process republishes, and it is the reference the setup documentation gives them.
Pinning a commit SHA here would freeze every customer's workflow on whatever commit happened to ship the day they copied the file, and they would never pick up a fix unless they edited it by hand.
Every third-party action in this PR is SHA-pinned — actions/checkout and both actions/upload-artifact steps. This is the one reference we publish and control.
The push carried the token as a global http.extraheader and pushed to the remote named `origin`. Both are decided by .git/config, which the agent has had write access to — `url.<base>.insteadOf` redirects `origin` to another host and a global header follows it there. The URL is now built from GITHUB_REPOSITORY, the push targets that URL, and the header is scoped to it. Also closed the config keys that make git run a program (core.sshCommand, core.pager, diff.external, gpg.program, commit.gpgsign, core.attributesFile) and disabled system/global config. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r execute Pushing to an explicit URL does not escape `url.<base>.insteadOf`, and `credential.helper` is a shell command git runs on a 401. Before touching the tree, read the repository's own config and refuse on any key that runs a program or rewrites a URL. The credential now travels in the environment rather than argv, which is world-readable via `ps`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Config is where git keeps every "run this program for me" hook, and it gains more each release. Listing the keys an ordinary actions/checkout writes and refusing everything else fails closed on keys that do not exist yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PATH directories on a runner are writable by the job's own user, which is the user the agent ran as. git is now resolved to an absolute path that this process cannot write to, and PATH is pinned rather than inherited. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"go" names no job, so the job comes from the proposal being approved. That assignment sat below the lookup that needs it, so asking worked and approving did not. Moved to where the workflow is final for both paths. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`git config --list` includes command-line config, so the check reported every GIT_SAFE_ARGS key as unexpected and refused to save. `--local` fixes it; the arguments are now shared between the code and a real-git test that fails without them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
…equest The agent's explanation was written to the result file only on a planning pass, so an rca answered into the void — the comment showed a table and no answer. And a read-only question had to be approved first, through a proposal that was empty because only test-pr writes a plan. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both from GitHub Advanced Security on this PR. The pinned tag is v4.6.2. The dynamic RegExp was already escaped by both callers; the escaping now lives in the helper, because an unescaped sha quietly matches OTHER commits rather than failing loudly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — six findings from code scanning, all addressed in Fixed:
|
Both dynamic regexes are gone. The proposal marker is one literal pattern with the sha captured and compared with ===, which also removes the bug the escaping existed to prevent: an unescaped sha quietly matched other commits. Naming a job compares words instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Finding 12 is the same line again — escaping the input doesn't satisfy the rule, only not interpolating does. Fair enough, so I've removed both dynamic patterns rather than argue it twice. Pushed in The proposal marker is now one literal pattern that captures the sha, compared with Naming a job (finding 8) now compares words instead of building Both escape helpers are deleted, because nothing needs escaping now. There is one 460 tests pass, including six added before the change — four pinning that a name is matched on whole words ( |
semgrep/ci is a required check and was failing on both. checkout is third party and now pinned to v4.4.0's commit; our own action stays on @v1 with a nosemgrep and the reason, because a floating major tag is how customers receive fixes without editing their copy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The rule id repeats its last segment; written once, it matched nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ting#3101 B1 a skip or a crash left the "in progress" check pending for ever, and a crash posted its check on the default branch instead of the PR head. B2 `stop` shared a concurrency group with the run it was sent to stop. B3 the App private key stayed in the environment after it had been used. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Identity on comments (another bot could get a proposal approved into a live run), the push no longer carries agent commits or whole untracked directories, persist-credentials off, one checkout path without --depth, approvals leave a trace, stop drops what it says it drops, and comments that are not for us no longer boot a runner. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…oved Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rca, verdict and fix all find the right build and then cannot read it, because gh has no credentials on a runner. The token is already in the job; it just never reached gh. Signing gh in directly needs no brain change and no CLI release. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A self-hosted runner outlives the job, so signing in to the shared gh config would leave the next job holding this repository's token. Scoped to a job directory and removed afterwards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gh auth status passed on the env var in that step, so the login was skipped and nothing was stored for the CLI to use. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gh refuses to store while GH_TOKEN is set, and the step ran under set -e so its failure skipped the whole run. Best effort now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Only `rca` and `verdict` ever showed what the agent concluded. Every other job posted a table saying "Succeeded" and nothing else — worst for `fix` in preview, where the prose IS the product: it found the cause, gave the exact one-line change, and the person who asked saw none of it without opening the job log. The text was never missing. The CLI emits an answer event for every verb; only its result FILE is narrowed to two. The Action spawned the CLI with stdout inherited, so the stream carrying the answer went to the log and nothing read it. This reads it on the way past, which needs no CLI change. The scanner copes with what a real pipe does: lines split across chunks, multi-byte characters split mid-character, CRLF, non-JSON output from the agent's own commands, and a runaway line with no newline, which is capped so a command printing megabytes cannot take the runner down. stdout is echoed onward before being parsed, so a long run still streams live. Comments are also capped at GitHub's 65,536 characters, which nothing enforced before. Over it the API rejects the call and the run reports nothing at all. The answer is cut first, pointing at the run page; the marker always survives, or every run posts a new comment instead of editing its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The answer and the failing-test names are the only parts of the comment this Action did not write — agent prose quoting build logs and repository files, and test names out of the run's report. On a fork's pull request that is attacker-influenceable, and even without malice a log line can happen to contain one of our markers. Markers are HTML comments, and they are how a later run finds this comment to edit and how a proposal's state is tracked, so one arriving inside borrowed text is read back as our own control data. Only the opening delimiter is broken: `<!--` still reads as `<!--` once rendered, so a quoted log line is unchanged for the reader. The size cap now measures the neutralised text, since escaping makes it longer and overrunning the limit stops the comment posting at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review finding on the previous commit. `line` sits beside `name` and `file` in the same report, carries the same trust, and went into the comment raw while its two siblings were neutralised. Same for the table's values, where the cases were saved, and why the run stopped. `line` is now a positive integer or nothing, which is stronger than escaping and is what it was always meant to be. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`test-pr`, proposing, is one of the verbs whose result file holds the same closing text the summary now shows. The proposal rendered the summary and then appended the result file below it, so the whole explanation appeared twice in one comment (run 41457337090). The plan is now left off when the summary already contains it, compared on squashed whitespace. `generate` keeps both, because its result file holds the case list rather than the prose summary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One comment, edited in place, keeps the thread tidy and loses track of what you are looking at. Where several people ask for different jobs on the same pull request, the comment quietly becomes the answer to whichever ran last, with nothing saying whose question that was. The report now carries an "Asked by" row linking the comment that started the run — the person's name as the link text, their comment as the target. A run nobody asked for has no such comment, and the row is left out rather than invented. This needed the comment's own URL, which the event payload has and the plan was discarding; only the author's login was kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A live run's first act is a dedupe claim — a real `git push` of a ref to origin — and it had nothing to authenticate with. The workflow sets `persist-credentials: false` (correct, and what the starter ships) and the environment allowlist keeps GH_TOKEN out, so git had no credential at all. `fix` and `generate` died in under a second at the claim, before any work. `test-pr`, `rca` and `verdict` have no claim and were unaffected, which is why it hid. git is now pointed at the credential gh already holds, for github.com only. No token enters any environment variable: git asks gh, and gh reads the job-scoped config dir the sign-in step created. The teardown is tested rather than trusted — the helper lives in the global git config, which outlives the job on a self-hosted runner and names a config dir the cleanup deletes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Replying `go` is spent by the run that carries the plan out. The marker saying so was written whatever happened — including to a run that never started. Found live (run 41463749519): an approved `fix` died in under a second because the dedupe claim had no credential. No work, nothing written, and the comment still said the plan was spent. Every later `go` on that commit was refused, a freshly posted proposal included, with no way out but a new commit. The question is whether a run happened, not whether it went well. Red tests and a ceiling did the work and may have written some of it, so they still spend it; a run that broke, could not start, or was skipped did not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Add the Test Companion action
Adds
test-companion/, a composite action that lets a team ask BrowserStackTest Companion for test work from a pull request comment.
Someone comments
@test-companion test this PR. It reads the change, posts thetests it would write, and writes them once a person replies
go. The resultcomes back as one comment and one check.
Jira: STM-3663 / STM-5434.
Why this repository
It is public and MIT, which is what customers need in order to reference the
action at all. Today it lives in a private repository, so nobody outside
BrowserStack can use it.
What is in here
test-companion/action.yml— the actiontest-companion/src/— 15 files, ~3,300 linestest-companion/test/— 374 tests, no network and no credentials neededtest-companion/README.md— customer-facing setuptest-companion/starter-workflow.yml— the file customers copyIt differs from the actions already here, in one good way
The four existing sub-actions are JavaScript actions with
dist/bundlescommitted by hand at 8–15MB. This one is a composite action with zero
dependencies and no bundle. What you see in
src/is what runs, and there isnothing to rebuild on release.
Two things this PR needs from maintainers
1. A floating
v1tag. This repository hasv1.0.0–v1.0.6and nov1.The README tells customers
@v1, which will not resolve until that tag exists.The current README elsewhere tells people
@master, which is unpinned — thesame hazard that bit us on 2026-10-29, when a point release changed behaviour
mid-pipeline. A floating
v1that moves on release is the fix.2. Agreement on review ownership. CODEOWNERS is
@browserstack/automate-public-repos. This action is owned and released by@browserstack/test-companion. Worth agreeing upfront whether that means aCODEOWNERS entry for this directory or a standing review arrangement, rather
than discovering it at the first urgent release.
What this does not do
It does not get a Marketplace listing. Marketplace needs a single
action.ymlat the repository root, and that issetup-env. Every sub-actionhere is currently unlisted (checked: all three 404). If a Marketplace entry is
a requirement, it needs a separate single-action repository.
Nothing proprietary is published here. The agent itself is in the
tcCLI,which the action downloads at runtime and which stays private. This is the
adapter between GitHub and that CLI.
Testing
374 passing. Beyond that, the action has been exercised on real GitHub runs
against a live repository: the comment flow, the approval gate, stopping a run,
closing a pull request mid-run, the oversized-change warning, writing tests and
committing them to the branch, and refusing a settings file planted on a pull
request branch.